Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: handle srcset local image paths with spaces #9537

Merged
merged 5 commits into from
Dec 28, 2023
Merged

fix: handle srcset local image paths with spaces #9537

merged 5 commits into from
Dec 28, 2023

Conversation

walter9388
Copy link
Contributor

Changes

fix #9535

srcset now replaces (spaces) with %20 (see issue #9535 for details).

Testing

Added testing which looks at the <img> element after build and checks srcset is correct.

Docs

N/A (bug fix)

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 27, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 4fe883e

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Dec 27, 2023
@walter9388
Copy link
Contributor Author

The following test is failing with the size being equal to 2 instead of 1:

astro/packages/astro/test/core-image.test.js

Lines 255 to 261 in b34bd2b

let local1x = $('#local-1x img');
expect(
new Set([
...parseSrcset(local1x.attr('srcset')).map((src) => src.url),
local1x.attr('src'),
]).size
).to.equal(1);

I undid my changes and found the test was still failing on main. It seems to be related to when densities = 1:

astro/packages/astro/test/fixtures/core-image/src/pages/srcset.astro

Lines 27 to 33 in b34bd2b

<div id="local-1x">
<!--
In this example, only one image should be generated :
- The base image, 1x density should be the same as the base image
-->
<Image src={image} width={image.width / 2} densities={[1]} alt="" />
</div>

I don't think this has anything to do with this PR?! Any insight into why this test is suddenly failiing?

@Princesseuh
Copy link
Member

No, the test failure is valid because it checks that srcset and src are the same URL, which with this change they're not anymore since one is encoded and the other isn't. I'll take a look!

@Princesseuh Princesseuh merged commit 16e61fc into withastro:main Dec 28, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Dec 28, 2023
Merged
@walter9388
Copy link
Contributor Author

No, the test failure is valid because it checks that srcset and src are the same URL, which with this change they're not anymore since one is encoded and the other isn't. I'll take a look!

Ah that makes sense. Thanks for all your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florian-lefebvre florian-lefebvre florian-lefebvre left review comments

@Princesseuh Princesseuh Princesseuh left review comments

@matthewp matthewp matthewp approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failed parsing 'srcset' attribute value since it has an unknown descriptor.
4 participants
@walter9388 @Princesseuh @matthewp @florian-lefebvre